feat: add diff and get command for the registry #1851
Conversation
severin-amrein
force-pushed
the
sev/dre-registry-diff
branch
2 times, most recently
from
9771903 to
6f57947
Compare
There was a problem hiding this comment.
General comment: dre registry is used in a lot of our tooling (that isn't all in this repo sadly) and changing its interface will demand a lot of additional work that we may not even see now. I suggest one of the following:
- Leave current
dre registryas is, extract the things that you need in a module and make your own new command for the diff - Somehow keep the old behavior as is
dre registry> dumps the registry
Personally I think that the first one is easier
I'll let @pietrodimarco-dfinity finish the review as he has more insights into what is needed. These are just styling comments.
severin-amrein
force-pushed
the
sev/dre-registry-diff
branch
from
3bdfa14 to
6693ae8
Compare
severin-amrein
force-pushed
the
sev/dre-registry-diff
branch
from
6693ae8 to
19985f6
Compare
| // Ensure local registry is initialized/synced to have content available | ||
| // (no-op in offline mode) | ||
| let _ = ctx.registry_with_version(None).await; | ||
| // Reorder if needed to ensure increasing order |
There was a problem hiding this comment.
Not sure reordering is expected by clients...I would panic if that's the case
| } else if version_1 > 0 && version_2 > 0 { | ||
| from_version = version_1_u64; | ||
| to_version = version_2_u64; | ||
| } else if version_1 * version_1 < 0 { |
There was a problem hiding this comment.
This should be version_1 * version_2 right?
| anyhow::bail!("Cannot mix positive version numbers and negative indices"); | ||
| } else if version_1 == 0 || version_2 == 0 { | ||
| anyhow::bail!("Version 0 is not supported"); | ||
| } else { |
There was a problem hiding this comment.
This is unreachable I believe. We can probably simplify:
f version_1 * version_2 < 0 {
anyhow::bail!("Cannot mix positive version numbers and negative indices");
} else if version_1 > 0 {
....
} else if version_1 < 0 {
....
} else
| ) | ||
| } | ||
|
|
||
| pub fn create_from_args( |
There was a problem hiding this comment.
A new here is probably ideal here. Instead of the enum VersionFillMode I would create two methods new_from_start and new_to_end or similar. With an additional method grouping common functionalities
| } | ||
|
|
||
| impl VersionRange { | ||
| pub fn get_from(&self) -> u64 { |
There was a problem hiding this comment.
I would just make them pub instead
3 participants
Backwards compatibility:
Introduction of three new subcommands:
Get: Show aggregated registry data
History: Show registry data of specified versions
Diff: Show diff of the data between two aggregated versions
Manual Testing: